MINIFICPP-2715 - Use symbols to check api compatibility#2105
MINIFICPP-2715 - Use symbols to check api compatibility#2105adamdebreceni wants to merge 5 commits intoapache:mainfrom
Conversation
Closes #2105 Signed-off-by: Martin Zink <martinzink@apache.org>
Closes #2105 Signed-off-by: Martin Zink <martinzink@apache.org>
There was a problem hiding this comment.
Pull request overview
This pull request refactors the extension compatibility checking mechanism from a file-based string search approach to a symbol-based approach. Instead of embedding build identifiers and API version strings in the binary and searching for them, the system now uses dynamic symbol resolution to verify compatibility.
Changes:
- C++ extensions now rely on unique symbol names (based on BUILD_IDENTIFIER) for compatibility checking, enforced through dynamic linking
- C extensions use explicit version checking via a
MinifiApiVersionsymbol - Removed file-based verification logic and associated tests
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| minifi-api/minifi-c-api.def | Added export ordinal for MinifiFlowFileGetAttributes function |
| minifi-api/include/minifi-c/minifi-c.h | Removed MINIFI_API_VERSION_TAG macro, added MINIFI_CREATE_EXTENSION_FN macro and helper macros, changed MinifiCreateExtension signature, added MinifiAgent typedef |
| minifi-api/CMakeLists.txt | Added compile definition to create unique extension function names based on BUILD_IDENTIFIER |
| libminifi/test/unit/ExtensionVerificationTests.cpp | Removed file-based extension verification tests |
| libminifi/src/minifi-c.cpp | Updated MinifiCreateExtension signature, added uniquely-named wrapper function, reordered includes |
| libminifi/src/core/extension/Utils.cpp | Removed file-based verification implementation |
| libminifi/src/core/extension/ExtensionManager.cpp | Added extension_being_initialized tracking, removed file-based verification calls |
| libminifi/src/core/extension/Extension.cpp | Replaced file-based verification with symbol lookup for extension type detection and C API version checking |
| libminifi/include/core/extension/Utils.h | Removed LibraryType enum and verification methods |
| libminifi/include/core/extension/ExtensionManager.h | Added getExtensionBeingInitialized static method |
| libminifi/include/core/extension/Extension.h | Added Version struct, added version() method, reordered includes |
| extensions/sftp/SFTPLoader.cpp | Renamed InitExtension to MinifiInitCppExtension, updated to use MinifiCreateCppExtension wrapper |
| extensions/sftp/CMakeLists.txt | Set HAS_CUSTOM_INITIALIZER property |
| extensions/python/pythonloader/PyProcLoader.cpp | Renamed InitExtension to MinifiInitCppExtension, updated to use MinifiCreateCppExtension wrapper |
| extensions/python/pythonlibloader/PythonLibLoader.cpp | Renamed InitExtension to MinifiInitCppExtension, updated to use MinifiCreateCppExtension wrapper |
| extensions/python/CMakeLists.txt | Set HAS_CUSTOM_INITIALIZER property for both Python extensions |
| extensions/opencv/OpenCVLoader.cpp | Renamed InitExtension to MinifiInitCppExtension, updated to use MinifiCreateCppExtension wrapper |
| extensions/opencv/CMakeLists.txt | Set HAS_CUSTOM_INITIALIZER property |
| extensions/llamacpp/processors/ExtensionInitializer.cpp | Added MinifiApiVersion symbol, renamed to MinifiInitExtension for C extension, removed MINIFI_API_VERSION_TAG usage |
| extensions/ExtensionInitializer.cpp | New generic extension initializer for extensions without custom initialization logic |
| extension-framework/include/utils/ExtensionInitUtils.h | Added MinifiCreateCppExtension helper function |
| cmake/Extensions.cmake | Removed build identifier file generation, added conditional ExtensionInitializer.cpp inclusion, removed get_build_id_variable_name function |
| Extensions.md | Updated documentation to describe both C and C++ extension approaches |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .processors_ptr = nullptr | ||
| }; | ||
| return MinifiCreateExtension(minifi::utils::toStringView(MINIFI_API_VERSION), &ext_create_info); | ||
| return minifi::utils::MinifiCreateCppExtension( &ext_create_info); |
There was a problem hiding this comment.
Extra space before the opening parenthesis in the function call. This should be MinifiCreateCppExtension(&ext_create_info) without the space, consistent with all other calls to this function in the codebase (see SFTPLoader.cpp:47, PythonLibLoader.cpp:117, OpenCVLoader.cpp:45, ExtensionInitializer.cpp:34).
| return minifi::utils::MinifiCreateCppExtension( &ext_create_info); | |
| return minifi::utils::MinifiCreateCppExtension(&ext_create_info); |
| .processors_ptr = nullptr | ||
| }; | ||
| return MinifiCreateExtension(minifi::utils::toStringView(MINIFI_API_VERSION), &ext_create_info); | ||
| return MinifiCreateCppExtension(&ext_create_info); |
There was a problem hiding this comment.
The function call should be minifi::utils::MinifiCreateCppExtension(&ext_create_info) to be consistent with the actual implementation. The MinifiCreateCppExtension function is in the minifi::utils namespace (see extension-framework/include/utils/ExtensionInitUtils.h:33-35), and all other C++ extension loaders use the fully qualified name (see SFTPLoader.cpp:47, PyProcLoader.cpp:52, PythonLibLoader.cpp:117, OpenCVLoader.cpp:45, ExtensionInitializer.cpp:34).
| return MinifiCreateCppExtension(&ext_create_info); | |
| return minifi::utils::MinifiCreateCppExtension(&ext_create_info); |
| PRIVATE "MODULE_NAME=${extension-name}" | ||
| PRIVATE "BUILD_ID_VARIABLE_NAME=${build-id-variable-name}") |
There was a problem hiding this comment.
The reference to build-id-variable-name on line 35 is problematic. This variable is never defined after the removal of the get_build_id_variable_name function. Since the BUILD_ID_VARIABLE_NAME compile definition is no longer used in the new symbol-based approach for API compatibility checking, this entire line (line 35) should be removed.
| PRIVATE "MODULE_NAME=${extension-name}" | |
| PRIVATE "BUILD_ID_VARIABLE_NAME=${build-id-variable-name}") | |
| PRIVATE "MODULE_NAME=${extension-name}") |
| bool Extension::load(bool global) { | ||
| dlerror(); | ||
| if (global) { | ||
| handle_ = dlopen(library_path_.string().c_str(), RTLD_NOW | RTLD_GLOBAL); // NOLINT(cppcoreguidelines-owning-memory) | ||
| } else { | ||
| handle_ = dlopen(library_path_.string().c_str(), RTLD_NOW | RTLD_LOCAL); // NOLINT(cppcoreguidelines-owning-memory) | ||
| } | ||
| if (!handle_) { | ||
| logger_->log_error("Failed to load extension '{}' at '{}': {}", name_, library_path_, dlerror()); | ||
| return false; | ||
| } else { | ||
| logger_->log_trace("Loaded extension '{}' at '{}'", name_, library_path_); | ||
| } | ||
| logger_->log_trace("Dlopen succeeded for extension '{}' at '{}'", name_, library_path_); | ||
| if (findSymbol("MinifiInitCppExtension")) { | ||
| logger_->log_trace("Loaded cpp extension '{}' at '{}'", name_, library_path_); | ||
| return true; | ||
| } | ||
| if (!findSymbol("MinifiInitExtension")) { | ||
| logger_->log_error("Failed to load c extension '{}' at '{}': No initializer found", name_, library_path_); | ||
| return false; | ||
| } | ||
| auto api_version = reinterpret_cast<const char* const*>(findSymbol("MinifiApiVersion")); | ||
| if (!api_version) { | ||
| logger_->log_error("Failed to load c extension '{}' at '{}': No MinifiApiVersion symbol found", name_, library_path_); | ||
| return false; | ||
| } | ||
| utils::SVMatch match; | ||
| if (!utils::regexSearch(*api_version, match, utils::Regex{R"(^([0-9]+)\.([0-9]+)\.([0-9]+)$)"})) { | ||
| logger_->log_error("Failed to load c extension '{}' at '{}': Api version is in invalid format: '{}'", name_, library_path_, *api_version); | ||
| return false; | ||
| } | ||
| gsl_Assert(match.size() == 4); | ||
| version_.major = std::stoi(match[1]); | ||
| version_.minor = std::stoi(match[2]); | ||
| version_.patch = std::stoi(match[3]); | ||
| if (version_.major != MINIFI_API_MAJOR_VERSION) { | ||
| logger_->log_error("Failed to load c extension '{}' at '{}': Api major version mismatch, application is '{}' while extension is '{}'", | ||
| name_, library_path_, MINIFI_API_VERSION, *api_version); | ||
| return false; | ||
| } | ||
| if (version_.minor > MINIFI_API_MINOR_VERSION) { | ||
| logger_->log_error("Failed to load c extension '{}' at '{}': Extension is built for a newer version, application is '{}' while extension is '{}'", | ||
| name_, library_path_, MINIFI_API_VERSION, *api_version); | ||
| return false; | ||
| } | ||
| logger_->log_debug("Loaded c extension '{}' at '{}': Application version is '{}', extension version is '{}'", | ||
| name_, library_path_, MINIFI_API_VERSION, *api_version); | ||
| return true; |
There was a problem hiding this comment.
The new symbol-based compatibility checking logic lacks test coverage. The previous test file (ExtensionVerificationTests.cpp) was removed without replacement tests. Consider adding tests that verify:
- C++ extensions with matching BUILD_IDENTIFIER can be loaded
- C++ extensions with non-matching BUILD_IDENTIFIER fail to load (symbol resolution failure)
- C extensions with compatible MinifiApiVersion can be loaded
- C extensions with incompatible major version are rejected
- C extensions with newer minor version are rejected
- C extensions without MinifiApiVersion symbol are rejected
|
|
||
| ## C extensions | ||
| You can build a shared library depending on the C capabilities of the agent as given in the `minifi-c.h` file. | ||
| For the shared library to be considered a valid extension, it has to have a global symbol with the name `MinifiCApiVersion` |
There was a problem hiding this comment.
The documentation states the symbol should be named MinifiCApiVersion, but the actual code in Extension.cpp line 70 looks for MinifiApiVersion (without the "C"). The documentation should be corrected to match the implementation.
| For the shared library to be considered a valid extension, it has to have a global symbol with the name `MinifiCApiVersion` | |
| For the shared library to be considered a valid extension, it has to have a global symbol with the name `MinifiApiVersion` |
martinzink
left a comment
There was a problem hiding this comment.
Works great, and the latest version will work with new api versions without changing code on rust binding side 👍
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically main)?
Is your initial contribution a single, squashed commit?
For code changes:
For documentation related changes:
Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.